Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
📝 WalkthroughWalkthroughadds rollback and named-backup restore capabilities. expands codex-cli sync state and APIs to capture rollback snapshots for manual applies, expose rollback planning and execution, integrate rollback controls into the sync center UI, and add named/rotating backup tooling in storage and extensive tests. Changes
Sequence Diagram(s)sequenceDiagram
participant user as user
participant ui as sync center ui
participant cli as codex-cli sync
participant storage as account storage
participant fs as filesystem
user->>ui: manual apply
activate ui
ui->>cli: applyCodexCliSyncToStorage(trigger: "manual")
activate cli
cli->>storage: apply changes
activate storage
storage->>fs: write accounts
fs-->>storage: ok
deactivate storage
cli->>cli: detect changes
alt changes detected
cli->>fs: captureRollbackSnapshot()
fs-->>cli: snapshot metadata (name,path)
cli->>fs: saveRollbackStorageWithRetry()
fs-->>cli: saved
end
cli-->>ui: CodexCliSyncRun { trigger, rollbackSnapshot }
deactivate cli
ui->>user: show run + rollback hint
user->>ui: request rollback
activate ui
ui->>cli: getLatestCodexCliSyncRollbackPlan()
activate cli
cli->>fs: findLatestManualRollbackRun()
fs-->>cli: run with snapshot
cli->>fs: loadRollbackSnapshot()
fs-->>cli: snapshot storage
cli-->>ui: plan ready with snapshot
deactivate cli
ui->>user: confirm rollback
user->>ui: confirm
ui->>cli: rollbackLatestCodexCliSync()
activate cli
cli->>storage: restoreFromSnapshot()
activate storage
storage->>fs: write restored accounts
fs-->>storage: ok
deactivate storage
cli-->>ui: rollback result (restored)
deactivate cli
ui->>ui: refresh preview and history
deactivate ui
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes concerns worth flagging:
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Adds manual rollback support for Codex CLI sync “sync-center” operations by capturing pre-apply snapshots, exposing rollback planning/restoration APIs, and wiring a rollback action into the settings UI/CLI with corresponding tests.
Changes:
- Capture and persist rollback checkpoint metadata for manual Codex CLI sync applies.
- Introduce rollback planning + restore APIs (
getLatestCodexCliSyncRollbackPlan,rollbackLatestCodexCliSync,rollbackLastCodexCliSync). - Add a rollback action (
L) to the sync-center UI and expand test coverage around checkpoint selection and error cases.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/codex-manager-cli.test.ts | Extends CLI mocks/reset paths to include new rollback APIs. |
| test/codex-cli-sync.test.ts | Adds tests for snapshot capture, rollback planning/restore, and error handling. |
| lib/ui/copy.ts | Updates sync-center help text and adds rollback action label. |
| lib/storage.ts | Adds a new snapshot reason for codex-cli sync checkpoints. |
| lib/codex-manager/settings-hub.ts | Wires rollback into sync-center menu + key handling and triggers “manual” apply runs. |
| lib/codex-cli/sync.ts | Implements rollback snapshot capture, plan resolution, and restore logic; augments sync run metadata. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function isManualChangedSyncRun(run: CodexCliSyncRun | null): run is CodexCliSyncRun { | ||
| return Boolean(run && run.outcome === "changed" && run.trigger === "manual"); | ||
| } | ||
|
|
||
| async function findLatestManualRollbackRun(): Promise< | ||
| CodexCliSyncRun | null | ||
| > { | ||
| const history = await readSyncHistory({ kind: "codex-cli-sync" }); | ||
| for (let index = history.length - 1; index >= 0; index -= 1) { | ||
| const entry = history[index]; | ||
| if (!entry || entry.kind !== "codex-cli-sync") continue; | ||
| const run = normalizeCodexCliSyncRun(entry.run); | ||
| if (isManualChangedSyncRun(run)) { | ||
| return run; | ||
| } | ||
| } | ||
| return null; | ||
| } |
lib/codex-cli/sync.ts
Outdated
| async function findLatestManualRollbackRun(): Promise< | ||
| CodexCliSyncRun | null | ||
| > { | ||
| const history = await readSyncHistory({ kind: "codex-cli-sync" }); |
| async function getSyncCenterRollbackPlan(): Promise<CodexCliSyncRollbackPlan> { | ||
| if ( | ||
| typeof codexCliSyncModule.getLatestCodexCliSyncRollbackPlan === "function" | ||
| ) { | ||
| return codexCliSyncModule.getLatestCodexCliSyncRollbackPlan(); | ||
| } | ||
| return { | ||
| status: "unavailable", | ||
| reason: "Rollback checkpoint is unavailable.", | ||
| snapshot: null, | ||
| }; | ||
| } | ||
|
|
||
| async function runSyncCenterRollback( | ||
| plan: CodexCliSyncRollbackPlan, | ||
| ): Promise<CodexCliSyncRollbackResult> { | ||
| if (typeof codexCliSyncModule.rollbackLatestCodexCliSync === "function") { | ||
| return codexCliSyncModule.rollbackLatestCodexCliSync(plan); | ||
| } | ||
| return { | ||
| status: "unavailable", | ||
| reason: "Rollback checkpoint is unavailable.", | ||
| snapshot: plan.snapshot, | ||
| }; | ||
| } |
lib/ui/copy.ts
Outdated
| syncCenterHelp: "Enter Select | A Apply | L Rollback | R Refresh | Q Back", | ||
| syncCenterOverviewHeading: "Sync Overview", | ||
| syncCenterActionsHeading: "Actions", | ||
| syncCenterRefresh: "Refresh Preview", | ||
| syncCenterApply: "Apply Preview To Target", | ||
| syncCenterRollback: "Rollback Last Apply", |
lib/ui/copy.ts
Outdated
| syncCenterActionsHeading: "Actions", | ||
| syncCenterRefresh: "Refresh Preview", | ||
| syncCenterApply: "Apply Preview To Target", | ||
| syncCenterRollback: "Rollback Last Apply", |
| expect(getLastCodexCliSyncRun()?.trigger).toBe("manual"); | ||
| expect(getLastCodexCliSyncRun()?.rollbackSnapshot).toBeNull(); | ||
| }); | ||
|
|
ndycode
force-pushed
the
fresh/14-auto-snapshots
branch
from
0fd695e to
97a02c2
Compare
ndycode
force-pushed
the
fresh/15-sync-rollback
branch
from
e522b04 to
1b45f34
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/codex-cli/sync.ts`:
- Around line 494-503: getLatestCodexCliSyncRollbackPlan currently relies only
on readSyncHistory() and can miss a just-appended checkpoint or throw; update it
to first consult getLastCodexCliSyncRun() (to pick up the most recent
in-memory/just-published run from publishCodexCliSyncRun()), and only then
attempt the history read; wrap the
readSyncHistory()/findLatestManualRollbackRun() logic in a try/catch so that any
history-read exception is swallowed and the function returns a closed failure {
status: "unavailable", reason: "...", snapshot: null } instead of letting the
rejection escape; keep using loadRollbackSnapshot() when a valid manual run is
found.
- Around line 362-372: captureRollbackSnapshot currently re-reads disk via
snapshotAccountStorage which races with concurrent saveAccounts; change
captureRollbackSnapshot to accept and use the in-memory baseline/accounts passed
from the reconciliation flow (e.g., add a parameter like currentBaseline or
accounts and update the caller that invokes captureRollbackSnapshot after
reconcile/save) and either (a) extend snapshotAccountStorage to accept an
accounts object to create metadata from memory, or (b) add a new
snapshotFromBaseline(accounts) helper used by captureRollbackSnapshot so the
rollback metadata reflects the exact manual apply state; ensure snapshot
creation is performed while holding the same storage lock used by saveAccounts
or implement a tight lock/atomic handoff to avoid races with saveAccounts, add
retries/backoff for filesystem EBUSY/HTTP 429 during snapshot write, and update
tests (test/codex-cli-sync.test.ts) to assert metadata and retry behavior; keep
logs free of tokens/emails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b4aeef97-8092-4039-8bb8-94c562b86e01
📒 Files selected for processing (6)
lib/codex-cli/sync.tslib/codex-manager/settings-hub.tslib/storage.tslib/ui/copy.tstest/codex-cli-sync.test.tstest/codex-manager-cli.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/codex-manager/settings-hub.tslib/storage.tslib/ui/copy.tslib/codex-cli/sync.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/codex-manager-cli.test.tstest/codex-cli-sync.test.ts
🔇 Additional comments (6)
lib/storage.ts (1)
212-213: test coverage for the new snapshot reason already exists.The "codex-cli-sync" snapshot reason is tested in lib/storage.ts:213. test/codex-cli-sync.test.ts includes explicit regression tests: snapshot metadata recording (test/codex-cli-sync.test.ts:1349), rollback retry on windows transient EBUSY (test/codex-cli-sync.test.ts:1840), and rollback retry on EPERM with logging validation (test/codex-cli-sync.test.ts:1912). No additional tests needed.
lib/ui/copy.ts (1)
131-136: rollback keybinding already has regression test coverage.the "l" hotkey for sync center rollback is already wired (
lib/codex-manager/settings-hub.ts:2855) and tested. rollback functions have vitest coverage intest/codex-cli-sync.test.ts:1349,1670,1760,1840,1912, and the cli-level keybinding is tested viatest/codex-manager-cli.test.ts:5662(triggerSettingsHotkey("l")). the rollback action is wrapped withwithQueuedRetry(lib/codex-manager/settings-hub.ts:2872) which serializes viaenqueueSettingsWrite, protecting against concurrent dispatch. retry handling covers 429 and filesystem errors (EBUSY) as required.> Likely an incorrect or invalid review comment.test/codex-manager-cli.test.ts (4)
35-36: mock declarations look good.properly declared as
vi.fn()and follow naming conventions.
162-165: mock wiring is correct.both
getLatestCodexCliSyncRollbackPlanandrollbackLatestCodexCliSyncare properly exposed in thevi.mockfor../lib/codex-cli/sync.js.
579-580: reset calls properly added to beforeEach.ensures clean state between tests.
765-774: rollback tests already exist in the test suite—no action required.comprehensive rollback tests are present in
test/codex-cli-sync.test.ts, not intest/codex-manager-cli.test.ts. the mocks attest/codex-manager-cli.test.ts:765-774correctly stub unavailable responses for test isolation. the actual unit tests cover the scenarios you mentioned:
- successful rollback plan retrieval and execution (
test/codex-cli-sync.test.ts:1349,test/codex-cli-sync.test.ts:1471)- transient failures with windows EBUSY retry behavior (
test/codex-cli-sync.test.ts:1840)- error handling and missing checkpoint detection (
test/codex-cli-sync.test.ts:1670,test/codex-cli-sync.test.ts:1760)- validation of rollback metadata (
test/codex-cli-sync.test.ts:1717)the mocking strategy in codex-manager-cli.test.ts is appropriate since integration with the rollback module is tested at the unit level.
> Likely an incorrect or invalid review comment.
ndycode
force-pushed
the
fresh/15-sync-rollback
branch
from
a60335b to
c5de884
Compare
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/quota-cache.ts (1)
235-249:⚠️ Potential issue | 🟠 Majorserialize cache saves and clears on the same path.
lib/quota-cache.ts:245-249can still rename a temp file ontoquota-cache.jsonafterclearQuotaCache()has already succeeded atlib/quota-cache.ts:277-300. during reset/recovery, a concurrentsaveQuotaCache()can therefore resurrect the cache after the wipe and make the clear result lie. please put save and clear behind the same in-flight lock, and add a vitest race regression next totest/quota-cache.test.ts:108-184.as per coding guidelines, "lib/**: focus on auth rotation, windows filesystem IO, and concurrency."
Also applies to: 277-300
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/quota-cache.ts` around lines 235 - 249, The save/clear race allows saveQuotaCache() to rename a temp file onto quota-cache.json after clearQuotaCache() has removed it; serialize both operations by introducing or reusing a single in‑flight mutex/lock used by saveQuotaCache and clearQuotaCache (e.g., an async Mutex or Promise-based lock referenced where getQuotaCachePath(), fs.rename/temp write and the clear logic run) so saves wait for clears and vice versa, ensure the lock is acquired before writing the temp file/rename and before deleting/truncating the cache, and add a Vitest race/regression test alongside test/quota-cache.test.ts (near the existing 108-184 tests) that runs concurrent saveQuotaCache() and clearQuotaCache() to assert that after clear returns the cache file does not reappear.lib/live-account-sync.ts (1)
295-301:⚠️ Potential issue | 🟠 Majordon't advance the observed mtime until the reload commits.
lib/live-account-sync.ts:298-301updateslastKnownMtimeMsbeforelib/live-account-sync.ts:337-368knows whether the reload succeeded. if the first reload for a change fails once, later polls see no delta and never retry the same on-disk update, so the watcher can stay stale until the file changes again. keep a separate pending mtime or only commitlastKnownMtimeMsafter a successful reload. please add a deterministic vitest regression next totest/live-account-sync.test.ts:225-272for “first reload fails, second poll retries without another write”.As per coding guidelines,
lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios.Also applies to: 337-368
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/live-account-sync.ts` around lines 295 - 301, pollOnce currently sets lastKnownMtimeMs immediately when it sees a change which prevents retry if the first reload fails; introduce a separate pendingReloadMtime (or similar) that pollOnce sets when scheduling a reload (instead of updating lastKnownMtimeMs), and only update lastKnownMtimeMs inside the reload commit path after a successful reload (the code around scheduleReload and the reload success handler referenced at lines ~337-368). Update the reload logic to clear pendingReloadMtime on success and to retry on transient EBUSY/429 errors, and add a deterministic vitest regression next to test/live-account-sync.test.ts:225-272 that simulates “first reload fails, second poll retries without another write” to validate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/features.md`:
- Around line 59-60: Update the "Preview-first sync center" capability line in
docs/features.md to explicitly call out "operator rollback" (e.g., "Shows
one-way Codex CLI sync results, operator rollback of last manual apply, and
rollback context before apply") so the docs match the rollback behavior
implemented in the settings hub rollback code path in settings-hub.ts (the
roll-back-last-manual-apply logic around the ~2822–2827 area); also add a short
upgrade note in the docs explaining the new rollback capability and mention any
new npm scripts or CLI flags required by the workflow change to satisfy the
docs/** guidelines.
In `@docs/README.md`:
- Around line 20-26: The docs README lacks mention of the new manual rollback
workflow; update the portal landing copy in README.md (the index list entries
such as getting-started.md, restore/sync entries) to add a short "rollback"
entry that describes the manual rollback flow, add a link to the rollback
reference page and the upgrade notes, and mention the new npm script/CLI flag
that implements the rollback (ensure the text references the rollback docs and
upgrade notes files and the CLI flag/script name used by the PR).
In `@docs/reference/settings.md`:
- Around line 98-103: The "Validation:" subsection currently lists developer npm
commands (`npm run typecheck`, `npm run build`, `npm test`); replace those with
user-facing validation instructions (e.g., `codex auth check`, `codex auth
forecast --live` or other CLI commands that validate settings) and move the npm
commands into a contributor/developer guide or a new "Developer validation"
section; update the "Validation:" heading content accordingly and ensure the
change is applied where the current `Validation:` block appears so readers see
user-facing checks first while maintainers can find the npm steps in contributor
docs.
In `@docs/upgrade.md`:
- Around line 65-69: Update docs/upgrade.md to note the new rollback behavior
added in this release: when the manual sync apply flow now exposes manual
rollback planning/restoration (the manual sync-apply logic in sync.ts)
unreadable rollback history will fail closed instead of bubbling a probe error;
add a pointer to the sync-center handler (the sync-center path in
settings-hub.ts) for operators to inspect and run rollbacks, and mention that
this change affects post-sync expectations and any npm scripts/workflows that
invoke manual syncs so upgrade notes reflect the new rollback
planning/restoration flow.
In `@lib/codex-cli/sync.ts`:
- Around line 477-532: The loadRollbackSnapshot function currently does a single
fs.readFile and returns "unavailable" on transient Windows errors; update
loadRollbackSnapshot to retry reads with exponential backoff (handle EBUSY and
EPERM plus typical transient codes) before failing closed, mirroring the
write-side hardening pattern used later in this file (see the write-retry block
around lines ~545-565) and keep getRedactedFilesystemErrorLabel usage for final
error messages; also add/modify tests in test/codex-cli-sync.test.ts to simulate
transient EBUSY/EPERM followed by success and a permanent failure, asserting the
retry succeeds in the transient case and still reports unavailable for permanent
errors.
- Around line 42-43: The rollback plan/result and global in-flight promise are
not tied to a storage target so restores can write to the wrong store if
getStoragePath() changes; update the CodexCliSyncRollbackPlan and
CodexCliSyncRollbackResult types to include a targetPath (normalized), populate
targetPath when creating plans, and before running the restore code in
restoreRollbackPlan (the logic around getStoragePath() at the block that
restores plan.storage) validate that current getStoragePath() matches
plan.targetPath and abort/error if not; replace the single global
inFlightCodexCliSyncRollbackPromise with a map keyed by normalized targetPath
(so concurrent restores per root are separate) and use that map for dedupe; add
a vitest regression that calls setStoragePathDirect() between plan creation and
restore to assert abort/validation behavior; ensure tests reference the changed
symbols and handle EBUSY/429 retry patterns per project concurrency guidelines.
- Around line 188-199: normalizePathForComparison currently converts backslashes
to slashes and collapses repeated slashes which destroys Windows UNC roots (e.g.
\\server\share → /server/share); modify normalizePathForComparison to preserve a
leading double-slash when the original path is a UNC path (detect by original
starting with "\\" or, after conversion, starting with "//") so you only
collapse interior repeated slashes but keep the UNC "//" prefix, maintain the
existing Windows lowercasing behavior in that case, and mirror the identical fix
for the duplicated helper in settings-hub.ts; add a vitest regression in
test/codex-cli-sync.test.ts that asserts UNC paths preserve the leading
double-slash and that comparisons/selectors behave as expected.
In `@lib/codex-manager.ts`:
- Around line 5156-5163: The try/catch around loadAccounts() currently swallows
errors and leaves currentStorage null which later gets passed into
assessNamedBackupRestore(), causing unreadable stores to be treated as empty;
change the behavior in the loadAccounts call so that on error you rethrow (or
propagate) the failure instead of setting currentStorage=null, and ensure
calling code (the restore preview/restore initiation path that calls
assessNamedBackupRestore) blocks restore actions when loadAccounts fails; add a
vitest regression that simulates an unreadable current storage (mock
loadAccounts to throw) and asserts that restore preview/restore is rejected and
no duplicate/current-account or max-account checks proceed; update any affected
test files to reference the new regression and document that new queues (if
introduced) must handle EBUSY and 429 backoff scenarios per existing
concurrency/IO guidelines.
In `@lib/destructive-actions.ts`:
- Around line 102-107: In deleteAccountAtIndex, validate options.index is an
integer >= 0 before using it: replace the .at(options.index) usage with a check
like Number.isInteger(options.index) && options.index >= 0 and then use direct
indexing (options.storage.accounts[options.index]); if the check fails return
null (or reject as per existing API), and keep the existing identity re-check
via findMatchingAccountIndex(); also add Vitest regression tests in
destructive-actions.test.ts covering negative (-1) and fractional (e.g., 1.8)
indices to ensure the function returns null / rejects as expected.
In `@lib/storage.ts`:
- Around line 2300-2314: The code is using loadAccounts() fallback instead of
the in-flight transaction snapshot, causing snapshotAccountStorage() to capture
stale on-disk state when options.storage is omitted; update the logic in the
block around transactionSnapshotContext.getStore() (referencing
transactionSnapshotContext, withAccountStorageTransaction,
snapshotAccountStorage, and options.storage) to prefer the transaction snapshot
payload (e.g., transactionState.storage or transactionState.active storage
object) over await loadAccounts(), so that currentStorage is set from the
transaction snapshot when present and only falls back to loadAccounts() when no
transaction snapshot exists; ensure any tests that exercise
withAccountStorageTransaction() are updated/added to cover the implicit-storage
branch.
In `@lib/sync-history.ts`:
- Around line 133-140: The tail-reading logic in readHistoryTail should be made
UTF-8 safe and Windows-CRLF aware: replace direct Buffer.toString usage in the
chunk loop with Node's StringDecoder to preserve incomplete multibyte sequences
across chunk boundaries and use the same line-splitting regex (/\r?\n/) as
trimHistoryFileIfNeeded when breaking combined text into lines (update the logic
around remainder, combined, and lines in readHistoryTail and ensure parseEntry
receives intact strings). Add EBUSY retry-with-backoff to readHistoryTail (match
the retry pattern used in storage.ts) so reads encountering EBUSY are
queued/retried instead of failing immediately. Finally add vitest regression
tests covering a non-ASCII (multibyte) message that straddles the chunk boundary
and a CRLF line-ending edge case to ensure the fixes work end-to-end.
In `@lib/ui/auth-menu.ts`:
- Around line 371-415: formatQuotaSummary currently returns raw
account.quotaSummary when structured segments are not built, which can leak
ANSI/control sequences; update formatQuotaSummary to pass the fallback
quotaSummary through the existing terminal sanitizer function (the sanitizer
added earlier in this file) before returning, and ensure any place that emits
the fallback uses the sanitized string; add a vitest unit test adjacent to
test/auth-menu-hotkeys.test.ts that supplies an ansi-tainted quotaSummary and
asserts the menu output contains the sanitized (control-sequence-free) text.
In `@test/accounts-edge.test.ts`:
- Around line 8-9: The new spy mocks mockCommitPendingCodexCliSyncRun and
mockCommitCodexCliSyncRunFailure are never asserted, so add deterministic
assertions and make the mocks return a non-null pendingRun: update the mocks in
test/accounts-edge.test.ts (locations where mockCommitPendingCodexCliSyncRun and
mockCommitCodexCliSyncRunFailure are defined and used) to return an object
representing pendingRun instead of undefined, then in the failure test assert
that commitCodexCliSyncRunFailure was called with that pendingRun and in the
happy-path test assert that commitPendingCodexCliSyncRun was called (and/or
commitCodexCliSyncRunFailure was not called); ensure these assertions cover the
save-failure regression and the success path to detect any breakage in
lib/accounts.ts functions that trigger those hooks.
In `@test/codex-cli-sync.test.ts`:
- Around line 611-615: The test relies on a 500ms mtime delta set by
utimes/writeFile (using accountsPath and targetStoragePath) which is flaky on
some filesystems; instead mock the timestamp/stat read path the same way as the
existing block around lines 883-1065 does (stub fs.stat or the module/function
that reads file mtimes in the sync code) so the selection logic sees
deterministic mtimes for sourceTime and targetTime; update the assertions to use
the mocked stat results and remove dependence on actual utimes/writeFile timing.
In `@test/recovery.test.ts`:
- Around line 49-65: The removeWithRetry function is duplicated; remove the
local async function removeWithRetry from this test and instead import the
shared helper (named removeWithRetry) from the existing helpers module
(./helpers/remove-with-retry.js) and update any local references to use that
imported symbol; ensure the import is added at the top of the file and the
duplicate function is deleted so the test uses the centralized implementation.
In `@test/storage-flagged.test.ts`:
- Around line 254-296: The test currently asserts that both "EPERM" and "EBUSY"
retry, but EPERM is only retryable on Windows in the implementation
(clearFlaggedAccounts), making the test flaky on non-win32 platforms; update the
test in storage-flagged.test.ts to be platform-aware by: keep a cross-platform
retry test for "EBUSY" (or "EAGAIN") only, and either skip or restrict the
"EPERM" case to when process.platform === "win32" (or use os.platform()), or
split into two it blocks where the EPERM test is wrapped in a platform guard;
reference the test block using the existing it.each and the
clearFlaggedAccounts/getFlaggedAccountsPath symbols so the behavior matches
lib/storage.ts.
---
Outside diff comments:
In `@lib/live-account-sync.ts`:
- Around line 295-301: pollOnce currently sets lastKnownMtimeMs immediately when
it sees a change which prevents retry if the first reload fails; introduce a
separate pendingReloadMtime (or similar) that pollOnce sets when scheduling a
reload (instead of updating lastKnownMtimeMs), and only update lastKnownMtimeMs
inside the reload commit path after a successful reload (the code around
scheduleReload and the reload success handler referenced at lines ~337-368).
Update the reload logic to clear pendingReloadMtime on success and to retry on
transient EBUSY/429 errors, and add a deterministic vitest regression next to
test/live-account-sync.test.ts:225-272 that simulates “first reload fails,
second poll retries without another write” to validate the change.
In `@lib/quota-cache.ts`:
- Around line 235-249: The save/clear race allows saveQuotaCache() to rename a
temp file onto quota-cache.json after clearQuotaCache() has removed it;
serialize both operations by introducing or reusing a single in‑flight
mutex/lock used by saveQuotaCache and clearQuotaCache (e.g., an async Mutex or
Promise-based lock referenced where getQuotaCachePath(), fs.rename/temp write
and the clear logic run) so saves wait for clears and vice versa, ensure the
lock is acquired before writing the temp file/rename and before
deleting/truncating the cache, and add a Vitest race/regression test alongside
test/quota-cache.test.ts (near the existing 108-184 tests) that runs concurrent
saveQuotaCache() and clearQuotaCache() to assert that after clear returns the
cache file does not reappear.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0ff53d45-3ed6-4099-a9e7-6e55e4261ac1
📒 Files selected for processing (43)
README.mddocs/README.mddocs/features.mddocs/getting-started.mddocs/index.mddocs/reference/commands.mddocs/reference/settings.mddocs/reference/storage-paths.mddocs/troubleshooting.mddocs/upgrade.mdindex.tslib/accounts.tslib/cli.tslib/codex-cli/sync.tslib/codex-manager.tslib/codex-manager/settings-hub.tslib/destructive-actions.tslib/index.tslib/live-account-sync.tslib/quota-cache.tslib/storage.tslib/sync-history.tslib/ui/auth-menu.tslib/ui/copy.tstest/accounts-edge.test.tstest/accounts-load-from-disk.test.tstest/accounts.test.tstest/auth-menu-hotkeys.test.tstest/cli-auth-menu.test.tstest/cli.test.tstest/codex-cli-sync.test.tstest/codex-manager-cli.test.tstest/destructive-actions.test.tstest/documentation.test.tstest/live-account-sync.test.tstest/quota-cache.test.tstest/recovery.test.tstest/release-main-prs-regression.test.tstest/settings-hub-utils.test.tstest/storage-flagged.test.tstest/storage-recovery-paths.test.tstest/storage.test.tstest/sync-history.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
docs/**
⚙️ CodeRabbit configuration file
keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.
Files:
docs/features.mddocs/getting-started.mddocs/reference/storage-paths.mddocs/reference/commands.mddocs/troubleshooting.mddocs/README.mddocs/upgrade.mddocs/reference/settings.mddocs/index.md
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/release-main-prs-regression.test.tstest/accounts-edge.test.tstest/settings-hub-utils.test.tstest/destructive-actions.test.tstest/live-account-sync.test.tstest/recovery.test.tstest/cli.test.tstest/documentation.test.tstest/quota-cache.test.tstest/accounts.test.tstest/cli-auth-menu.test.tstest/auth-menu-hotkeys.test.tstest/storage-recovery-paths.test.tstest/storage-flagged.test.tstest/codex-cli-sync.test.tstest/accounts-load-from-disk.test.ts
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/index.tslib/quota-cache.tslib/codex-manager.tslib/cli.tslib/destructive-actions.tslib/codex-manager/settings-hub.tslib/ui/auth-menu.tslib/sync-history.tslib/codex-cli/sync.tslib/accounts.tslib/live-account-sync.tslib/storage.tslib/ui/copy.ts
🪛 LanguageTool
docs/getting-started.md
[style] ~77-~77: To make your writing flow more naturally, try moving the adverb ‘already’ closer to the verb ‘named’.
Context: ...rt Fresh Use the restore path when you already have named backup files and want to recover accoun...
(PERF_TENS_ADV_PLACEMENT)
🔇 Additional comments (46)
docs/reference/storage-paths.md (3)
25-25: lgtm - named backup paths documented consistently.both global and project-scoped named backup paths are now documented, aligning with the
lib/storage.tsbackup infrastructure.Also applies to: 60-60
105-114: restore workflow section looks good.documents the interactive restore flow and the new
codex auth restore-backupentrypoint. matches the pr objectives for manual rollback support.
131-131: verification commands updated.adding
codex auth loginas the first verification command makes sense since it's the primary entry point for restore/sync flows now.docs/index.md (1)
3-3: docs index updated to reflect new interactive flows.the expanded description, restore prompt note, and interactive workflows section accurately document the new dashboard paths for backup restore, sync preview/apply, and settings split. consistent with pr
#114changes.Also applies to: 15-16, 44-49
test/storage-recovery-paths.test.ts (3)
628-665: test properly distinguishes stale vs fresh artifacts.setting mtime to epoch (line 646) ensures stale artifacts are cleaned up, while the fresh artifact (lines 649-651) remains untouched. aligns with
lib/storage.ts:50whereROTATING_BACKUP_STALE_ARTIFACT_MAX_AGE_MS = 60_000.
727-750: timestamp manipulation for freshness checks looks correct.using relative timestamps (-5_000 for cache, +5_000 for manual checkpoint) ensures deterministic ordering for the latest valid backup selection test.
788-792: newest backup selection test uses proper time bracketing.
baseMtimeMs = Date.now() - 60_000with a 10-second offset between older and newer ensures the test reliably identifies the newest manual backup path.docs/reference/commands.md (3)
23-23: new commands documented correctly.
codex auth logindescription updated to reflect expanded entry points, andcodex auth restore-backupadded as new public command. matches the api surface exposed by pr#114.Also applies to: 29-29
104-111: workflow packs section provides clear navigation.documents the interactive dashboard workflows including startup recovery prompt, sync preview/apply, and settings split paths. consistent with
lib/codex-manager/settings-hub.tssync center flow.
125-129: repair and recovery examples updated.
codex auth restore-backupincluded in example block alongside existing fix/doctor commands.test/accounts-load-from-disk.test.ts (3)
4-10: hoisted mocks follow vitest best practices.using
vi.hoisted()ensures mocks are available before module imports, which is required for proper module mocking in vitest.
56-105: success path correctly tests commit sequencing.test verifies that when
saveAccountssucceeds,commitPendingCodexCliSyncRun(pendingRun)is called andcommitCodexCliSyncRunFailureis not. aligns withlib/accounts.ts:123-124.
107-151: failure path correctly tests error propagation.test verifies that when
saveAccountsrejects,commitCodexCliSyncRunFailure(pendingRun, saveError)is called with the actual error object. aligns withlib/accounts.ts:126.docs/getting-started.md (3)
52-53: restore prompt note documented.accurately describes the interactive restore prompt behavior when zero saved accounts and recoverable backups exist. notes suppression in non-interactive flows.
75-84: restore or start fresh section looks good.documents automatic and manual restore paths, backup location (
~/.codex/multi-auth/backups/<name>.json), and restore manager preview behavior.
87-95: sync and settings section documents new flows.covers the settings split (everyday vs advanced), codex cli sync preview/apply, and destination-only preservation. aligns with
lib/codex-manager/settings-hub.tssync center.test/cli-auth-menu.test.ts (3)
293-295: updated cancellation message matches production.the expected message "\nDelete saved accounts cancelled.\n" aligns with
lib/cli.ts:216.
297-322: reset-all flow tests cover both confirmation and cancellation paths.
- confirmation test verifies
mode: "reset"returned when user types "RESET".- cancellation test verifies "\nReset local state cancelled.\n" logged when confirmation fails.
- both tests verify
mockRl.close()called appropriately.aligns with
lib/cli.ts:223for cancellation message.
336-336: fresh action cancellation uses consistent message.same message as delete-all cancellation, which is expected since both are destructive delete operations.
test/recovery.test.ts (4)
164-328: override path tests for getActionableNamedBackupRestores look comprehensive.covers injected backups/assessor, explicit null currentStorage passthrough, and error isolation when assessment throws. good coverage of the override api surface.
330-549: storage-backed path tests properly cover windows filesystem errors.tests at lines 380-396, 398-471, and 473-549 verify EBUSY/EPERM handling during backup scanning and metadata stat operations. this aligns with the coding guidelines requirement to test windows filesystem behavior.
715-797: default assessment EBUSY retry behavior tested.test verifies 5 retry attempts for locked backup path (line 795:
expect(readPaths.filter((path) => path === lockedBackup?.path)).toHaveLength(5)), confirming retry logic works as expected.
801-833: resolveStartupRecoveryAction tests cover edge cases.
- line 808-812: verifies re-entry to empty storage menu when backups exist but none actionable.
- line 826-831: verifies oauth fallthrough when startup recovery scan fails.
good coverage of the decision logic in
lib/codex-manager.js.docs/reference/settings.md (1)
77-96: codex cli sync documentation looks good and aligns with pr objectives.the sync center documentation at
docs/reference/settings.md:77-96correctly describes the preview-first workflow, backup context (.bak,.bak.1,.bak.2,.wal), and one-way sync behavior. this matches the implementation inlib/codex-manager/settings-hub.tsandlib/codex-cli/sync.ts.lib/accounts.ts (2)
116-130: commit helper integration looks correct with proper ordering.the changes at
lib/accounts.ts:123-125correctly order the operations:
saveAccountspersists storage firstcommitPendingCodexCliSyncRunpublishes success only after persistencecommitCodexCliSyncRunFailurecaptures failures in the error pathboth commit functions have null guards per context snippets at
lib/codex-cli/sync.ts:346andlib/codex-cli/sync.ts:366, so calling them withsynced.pendingRun(which is currently always null perlib/codex-cli/sync.ts:1289,1314) is safe.
25-29: imports correctly updated for new commit helpers.the new imports at
lib/accounts.ts:25-29align with the expanded public api fromlib/codex-cli/sync.ts. tests attest/accounts.test.tsmock these functions and verify ordering/failure behavior.docs/troubleshooting.md (3)
62-70: backup restore problems table covers key failure modes.the new troubleshooting section at
docs/troubleshooting.md:62-70documents the expected failure scenarios for backup restore: missing backups, non-interactive terminal, invalid backups, and deduplication behavior. this aligns with the fail-closed behavior mentioned in pr objectives.
83-93: codex cli sync problems section correctly documents preview behavior and rollback context.the sync problems table at
docs/troubleshooting.md:83-93accurately describes:
- one-way sync is shipped behavior (not a bug)
- destination-only accounts are preserved
- storage backups must be enabled for rollback context
this aligns with the
lib/codex-cli/sync.tsimplementation where destination-only accounts are preserved rather than deleted.
125-171: reset options section provides clear manual equivalents.the reset options at
docs/troubleshooting.md:125-171document exact effects and manual file removal commands for both windows (powershell) and bash. the .wal and .bak* patterns are correctly included for account file cleanup.test/settings-hub-utils.test.ts (3)
375-416: windows path handling test covers case-insensitive matching.the test at
test/settings-hub-utils.test.ts:375-416correctly validates windows-style paths likeC:\\Users\\Neil\\.codex\\Accounts.jsonagainst posix-style context paths likec:/users/neil/.codex/accounts.json. this addresses the windows filesystem edge case requirement. good coverage.
498-618: retry tests cover EBUSY, EPERM, and 429 scenarios.the retry tests at
test/settings-hub-utils.test.ts:498-618verify:
loadAccountsretries on EBUSY (lines 498-536)loadCodexCliStateretries on EBUSY (lines 538-574)loadCodexCliStateretries on 429 withretryAfterMs(lines 576-618)these tests address the windows filesystem and rate-limit resilience requirements from the coding guidelines.
248-307: sync center overview test validates rollback context rendering.the test at
test/settings-hub-utils.test.ts:248-307verifies thebuildSyncCenterOverviewfunction renders:
- status, source path, auth path
- live watcher state
- preview summary (add/update/preserve counts)
- backup context with
.bakand.walpathsthis covers the ui rendering for rollback context described in the pr objectives.
test/accounts.test.ts (3)
243-298: test verifies commit ordering after successful save.the test at
test/accounts.test.ts:243-298correctly verifies:
saveAccountsis called beforecommitPendingCodexCliSyncRun(line 294-296)commitCodexCliSyncRunFailureMockis not called on success (line 297)good use of
invocationCallOrderto assert ordering guarantees.
300-354: test verifies failure path records sync-run failures correctly.the test at
test/accounts.test.ts:300-354verifies that whensaveAccountsthrows:
commitPendingCodexCliSyncRunis NOT called (line 349)commitCodexCliSyncRunFailureIS called with the pending run and error (lines 350-353)this regression test reproduces the failure scenario described in the pr objectives.
56-72: beforeEach correctly resets all mocks and sets default behaviors.the mock setup at
test/accounts.test.ts:56-72properly resets mocks and provides sensible defaults:
loadAccountsMockreturns null (no storage)syncAccountStorageFromCodexCliMockreturns unchanged state with null pendingRunloadCodexCliStateMockreturns nullthis ensures tests are isolated and deterministic.
lib/cli.ts (4)
29-31: isInteractiveLoginMenuAvailable correctly combines tty checks.the new helper at
lib/cli.ts:29-31properly combinesisNonInteractiveMode()withisTTY()to gate interactive menu availability. this is used at line 266 to determine whether to show the full interactive menu or fall back to readline prompts.
166-176: promptResetTypedConfirm requires explicit "RESET" confirmation.the reset confirmation at
lib/cli.ts:166-176usesDESTRUCTIVE_ACTION_COPY.resetLocalState.typedConfirmfor the prompt and requires exact "RESET" input. this matches the pattern used forpromptDeleteAllTypedConfirmat lines 154-164.
291-296: reset-all action correctly maps to "reset" login mode.the menu action handling at
lib/cli.ts:291-296correctly:
- prompts for typed confirmation via
promptResetTypedConfirm()- continues loop if cancelled (line 293-295)
- returns
{ mode: "reset" }on confirmationthis aligns with the
AuthMenuActiontype atlib/ui/auth-menu.ts:57-75where the action type is"reset-all"but maps to login mode"reset".
146-152: warnUnresolvableAccountSelection does not leak sensitive data.the warning function at
lib/cli.ts:146-152only logs email, accountId, or index - no refresh tokens or access tokens are exposed. this aligns with the coding guidelines requirement to check for logging that leaks tokens or emails.README.md (2)
115-120: interactive dashboard paths section documents new rollback flows.the new section at
README.md:115-120correctly documents:
codex auth login->Restore From Backupfor named backup restorecodex auth login->Settings->Codex CLI Syncfor sync previewcodex auth login->Settings->Everyday Settingsfor dashboard preferencesthis aligns with the sync center and rollback capabilities introduced in pr
#114.
19-20: what you get section updated for new capabilities.the feature list at
README.md:19-20now mentions "Interactive dashboard for login, restore, switching, sync preview, and settings" and the productized settings split. this reflects the expanded sync center and settings organization.test/destructive-actions.test.ts (5)
55-81: transaction mock correctly simulates rollback behavior.the mock at
test/destructive-actions.test.ts:55-81accurately simulateswithAccountAndFlaggedStorageTransaction:
- captures previous snapshot before handler (line 57)
- calls handler with current storage and persist function (line 58-78)
- on flagged save failure, attempts rollback (lines 66-70)
- throws
AggregateErrorif both flagged save and rollback fail (lines 71-74)this matches the production behavior in
lib/storage.tstransactional wrappers.
139-197: test validates index rebasing after account deletion.the test at
test/destructive-actions.test.ts:139-197verifies:
- deleting account at index 0 shifts remaining accounts (lines 182-185)
activeIndexrebases from 1 to 0 (line 186)activeIndexByFamilyrebases: codex 2→1, gpt-5.x 1→0 (lines 187-190)- snapshot is created before save (lines 177-181)
good regression coverage for the rebase logic in
lib/destructive-actions.ts.
256-297: test covers refresh token collision edge case.the test at
test/destructive-actions.test.ts:256-297verifies that when two accounts share the same refresh token but have different emails/accountIds, the correct account is deleted based on full identity matching. this testsfindMatchingAccountIndexbehavior inlib/destructive-actions.ts:121.
401-463: test validates aggregateerror when both saves fail.the test at
test/destructive-actions.test.ts:401-463verifies that when:
- flagged save fails (line 411)
- rollback save also fails (line 410)
an
AggregateErroris thrown containing both errors (lines 456-459). this ensures error information is preserved for debugging partial failures.
122-137: test verifies cli state not cleared on reset failure.the test at
test/destructive-actions.test.ts:122-137verifies that whenclearFlaggedAccountsthrows,clearCodexCliStateCacheis not called (line 136). this ensures partial reset failures don't leave cli state in an inconsistent state.
| | Productized settings split | Keeps `Everyday Settings` separate from `Advanced & Operator` controls | | ||
| | Preview-first sync center | Shows one-way Codex CLI sync results and rollback context before apply | |
There was a problem hiding this comment.
call out rollback as an operator action here.
docs/features.md:59-60 only mentions rollback context before apply, but the shipped flow in lib/codex-manager/settings-hub.ts:2822-2827 can also roll back the last manual apply. please name rollback explicitly in the capability map so the docs match the actual sync-center workflow.
As per coding guidelines, "docs/**: keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/features.md` around lines 59 - 60, Update the "Preview-first sync
center" capability line in docs/features.md to explicitly call out "operator
rollback" (e.g., "Shows one-way Codex CLI sync results, operator rollback of
last manual apply, and rollback context before apply") so the docs match the
rollback behavior implemented in the settings hub rollback code path in
settings-hub.ts (the roll-back-last-manual-apply logic around the ~2822–2827
area); also add a short upgrade note in the docs explaining the new rollback
capability and mention any new npm scripts or CLI flags required by the workflow
change to satisfy the docs/** guidelines.
| | [index.md](index.md) | Daily-use landing page for login, restore, sync, and diagnostics workflows | | ||
| | [getting-started.md](getting-started.md) | Install, first login, startup restore prompt, and first health check | | ||
| | [faq.md](faq.md) | Short answers to common adoption questions | | ||
| | [architecture.md](architecture.md) | Public system overview of the wrapper, storage, and optional plugin runtime | | ||
| | [features.md](features.md) | User-facing capability map | | ||
| | [features.md](features.md) | User-facing capability map, including backup restore, sync center, and settings split | | ||
| | [configuration.md](configuration.md) | Stable defaults, precedence, and environment overrides | | ||
| | [troubleshooting.md](troubleshooting.md) | Recovery playbooks for install, login, switching, and stale state | | ||
| | [troubleshooting.md](troubleshooting.md) | Recovery playbooks for install, login, restore, sync, and stale state | |
There was a problem hiding this comment.
document the rollback flow in the portal index.
docs/README.md:20-26 and docs/README.md:43-44 widen the restore/sync wording, but they still do not mention the new manual rollback workflow from this pr. that leaves the docs landing page out of sync with the changed cli surface and gives users no obvious path to the rollback docs or upgrade notes. please add rollback wording here and point to the matching reference/upgrade pages.
as per coding guidelines, "docs/**: keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts."
Also applies to: 43-44
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/README.md` around lines 20 - 26, The docs README lacks mention of the
new manual rollback workflow; update the portal landing copy in README.md (the
index list entries such as getting-started.md, restore/sync entries) to add a
short "rollback" entry that describes the manual rollback flow, add a link to
the rollback reference page and the upgrade notes, and mention the new npm
script/CLI flag that implements the rollback (ensure the text references the
rollback docs and upgrade notes files and the CLI flag/script name used by the
PR).
| Validation: | ||
|
|
||
| - `npm run typecheck` | ||
| - `npm run build` | ||
| - `npm test` | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
validation section includes dev commands rather than user-facing validation.
the validation subsection at lines 98-103 references npm run typecheck, npm run build, and npm test. these are developer/maintainer commands, not user-facing validation steps. the settings reference should document user-facing validation like codex auth check or codex auth forecast --live.
consider moving these to a contributor guide or replacing with user-facing commands:
📝 suggested fix
-Validation:
-
-- `npm run typecheck`
-- `npm run build`
-- `npm test`
+Validation:
+
+- `codex auth status`
+- `codex auth check`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Validation: | |
| - `npm run typecheck` | |
| - `npm run build` | |
| - `npm test` | |
| Validation: | |
| - `codex auth status` | |
| - `codex auth check` | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/reference/settings.md` around lines 98 - 103, The "Validation:"
subsection currently lists developer npm commands (`npm run typecheck`, `npm run
build`, `npm test`); replace those with user-facing validation instructions
(e.g., `codex auth check`, `codex auth forecast --live` or other CLI commands
that validate settings) and move the npm commands into a contributor/developer
guide or a new "Developer validation" section; update the "Validation:" heading
content accordingly and ensure the change is applied where the current
`Validation:` block appears so readers see user-facing checks first while
maintainers can find the npm steps in contributor docs.
| ### Startup Recovery Prompt | ||
|
|
||
| Interactive `codex auth login` now offers named-backup recovery before OAuth only when the session starts with zero saved accounts and at least one recoverable named backup. | ||
|
|
||
| The prompt is intentionally skipped in fallback/non-interactive login paths and after same-session `fresh` or `reset` actions so an intentional wipe does not immediately re-offer restore state. |
There was a problem hiding this comment.
the upgrade guide still misses the rollback behavior change.
docs/upgrade.md:65-69 documents the startup recovery prompt, but this release also changes what operators should expect after a manual sync apply: lib/codex-cli/sync.ts:567-661 now exposes manual rollback planning/restoration, and unreadable rollback history fails closed instead of bubbling a probe error. please add that note and point readers to the sync-center path in lib/codex-manager/settings-hub.ts:2815-2827.
As per coding guidelines, "docs/**: keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/upgrade.md` around lines 65 - 69, Update docs/upgrade.md to note the new
rollback behavior added in this release: when the manual sync apply flow now
exposes manual rollback planning/restoration (the manual sync-apply logic in
sync.ts) unreadable rollback history will fail closed instead of bubbling a
probe error; add a pointer to the sync-center handler (the sync-center path in
settings-hub.ts) for operators to inspect and run rollbacks, and mention that
this change affects post-sync expectations and any npm scripts/workflows that
invoke manual syncs so upgrade notes reflect the new rollback
planning/restoration flow.
| const result = await deleteSavedAccounts(); | ||
| invalidateAccountManagerCache(); | ||
| console.log( | ||
| "\nCleared saved accounts from active storage. Recovery snapshots remain available. Starting fresh.\n", | ||
| `\n${ | ||
| result.accountsCleared | ||
| ? DESTRUCTIVE_ACTION_COPY.deleteSavedAccounts.completed | ||
| : "Delete saved accounts completed with warnings. Some saved account artifacts could not be removed; see logs." | ||
| }\n`, | ||
| ); | ||
| } | ||
| break; | ||
| } | ||
|
|
||
| if (menuResult.mode === "reset") { | ||
| startFresh = true; | ||
| const result = await resetLocalState(); | ||
| invalidateAccountManagerCache(); | ||
| console.log( | ||
| `\n${ | ||
| result.accountsCleared && | ||
| result.flaggedCleared && | ||
| result.quotaCacheCleared | ||
| ? DESTRUCTIVE_ACTION_COPY.resetLocalState.completed | ||
| : "Reset local state completed with warnings. Some local artifacts could not be removed; see logs." | ||
| }\n`, | ||
| ); | ||
| break; | ||
| } |
There was a problem hiding this comment.
wire restore-backup before falling out of the menu loop.
lib/ui/auth-menu.ts:666-669 now exposes restore-backup, and test/cli.test.ts:707-729 asserts that promptLoginMode() can return { mode: "restore-backup" }. this dispatch block only handles fresh and reset among the new recovery modes, so restore-backup falls through to the default break at index.ts:3178-3180 and starts oauth login instead of recovery. please add an explicit handler here, and add an authorize-flow vitest regression so this wiring cannot drift again.
| function formatQuotaSummary( | ||
| account: AccountInfo, | ||
| ui: ReturnType<typeof getUiRuntimeOptions>, | ||
| ): string { | ||
| const summary = account.quotaSummary ?? ""; | ||
| const showCooldown = account.showQuotaCooldown !== false; | ||
| const left5h = normalizeQuotaPercent(account.quota5hLeftPercent) ?? parseLeftPercentFromSummary(summary, "5h"); | ||
| const left7d = normalizeQuotaPercent(account.quota7dLeftPercent) ?? parseLeftPercentFromSummary(summary, "7d"); | ||
| const left5h = | ||
| normalizeQuotaPercent(account.quota5hLeftPercent) ?? | ||
| parseLeftPercentFromSummary(summary, "5h"); | ||
| const left7d = | ||
| normalizeQuotaPercent(account.quota7dLeftPercent) ?? | ||
| parseLeftPercentFromSummary(summary, "7d"); | ||
| const segments: string[] = []; | ||
|
|
||
| if (left5h !== null || typeof account.quota5hResetAtMs === "number") { | ||
| segments.push(formatQuotaWindow("5h", left5h, account.quota5hResetAtMs, showCooldown, ui)); | ||
| segments.push( | ||
| formatQuotaWindow( | ||
| "5h", | ||
| left5h, | ||
| account.quota5hResetAtMs, | ||
| showCooldown, | ||
| ui, | ||
| ), | ||
| ); | ||
| } | ||
| if (left7d !== null || typeof account.quota7dResetAtMs === "number") { | ||
| segments.push(formatQuotaWindow("7d", left7d, account.quota7dResetAtMs, showCooldown, ui)); | ||
| segments.push( | ||
| formatQuotaWindow( | ||
| "7d", | ||
| left7d, | ||
| account.quota7dResetAtMs, | ||
| showCooldown, | ||
| ui, | ||
| ), | ||
| ); | ||
| } | ||
| if (account.quotaRateLimited || summary.toLowerCase().includes("rate-limited")) { | ||
| segments.push(ui.v2Enabled ? paintUiText(ui, "rate-limited", "danger") : `${ANSI.red}rate-limited${ANSI.reset}`); | ||
| if ( | ||
| account.quotaRateLimited || | ||
| summary.toLowerCase().includes("rate-limited") | ||
| ) { | ||
| segments.push( | ||
| ui.v2Enabled | ||
| ? paintUiText(ui, "rate-limited", "danger") | ||
| : `${ANSI.red}rate-limited${ANSI.reset}`, | ||
| ); |
There was a problem hiding this comment.
sanitize the raw quota-summary fallback too.
lib/ui/auth-menu.ts:100-106 adds the new terminal sanitizer, but formatQuotaSummary() still falls back to raw account.quotaSummary at lib/ui/auth-menu.ts:418-424 when it cannot build structured segments. that means an ansi/control sequence stored in quota data still reaches the menu. please sanitize that fallback and add a vitest case next to test/auth-menu-hotkeys.test.ts:260-302 for an ansi-tainted quotaSummary.
possible fix
if (segments.length === 0) {
- if (!summary) return "";
- return ui.v2Enabled ? paintUiText(ui, summary, "muted") : summary;
+ const safeSummary = sanitizeTerminalText(summary);
+ if (!safeSummary) return "";
+ return ui.v2Enabled ? paintUiText(ui, safeSummary, "muted") : safeSummary;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ui/auth-menu.ts` around lines 371 - 415, formatQuotaSummary currently
returns raw account.quotaSummary when structured segments are not built, which
can leak ANSI/control sequences; update formatQuotaSummary to pass the fallback
quotaSummary through the existing terminal sanitizer function (the sanitizer
added earlier in this file) before returning, and ensure any place that emits
the fallback uses the sanitized string; add a vitest unit test adjacent to
test/auth-menu-hotkeys.test.ts that supplies an ansi-tainted quotaSummary and
asserts the menu output contains the sanitized (control-sequence-free) text.
| const mockCommitPendingCodexCliSyncRun = vi.fn(); | ||
| const mockCommitCodexCliSyncRunFailure = vi.fn(); |
There was a problem hiding this comment.
the new sync-run spies are not asserted anywhere.
The added spies at test/accounts-edge.test.ts:8-9, test/accounts-edge.test.ts:32-33, and test/accounts-edge.test.ts:86-87 never verify the new bookkeeping in lib/accounts.ts:116-130. the save-failure regression at test/accounts-edge.test.ts:108-125 still passes with pendingRun missing, so a break where commitCodexCliSyncRunFailure() stops firing would go unnoticed. please return a non-null pendingRun from the mock and assert the failure hook here, plus the success hook in a happy-path case.
As per coding guidelines, "test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions."
Also applies to: 32-33, 86-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/accounts-edge.test.ts` around lines 8 - 9, The new spy mocks
mockCommitPendingCodexCliSyncRun and mockCommitCodexCliSyncRunFailure are never
asserted, so add deterministic assertions and make the mocks return a non-null
pendingRun: update the mocks in test/accounts-edge.test.ts (locations where
mockCommitPendingCodexCliSyncRun and mockCommitCodexCliSyncRunFailure are
defined and used) to return an object representing pendingRun instead of
undefined, then in the failure test assert that commitCodexCliSyncRunFailure was
called with that pendingRun and in the happy-path test assert that
commitPendingCodexCliSyncRun was called (and/or commitCodexCliSyncRunFailure was
not called); ensure these assertions cover the save-failure regression and the
success path to detect any breakage in lib/accounts.ts functions that trigger
those hooks.
| const sourceTime = new Date("2026-03-13T00:00:00.000Z"); | ||
| const targetTime = new Date("2026-03-13T00:00:00.500Z"); | ||
| await utimes(accountsPath, sourceTime, sourceTime); | ||
| await writeFile(targetStoragePath, "{\"version\":3}", "utf-8"); | ||
| await utimes(targetStoragePath, targetTime, targetTime); |
There was a problem hiding this comment.
stub the timestamp source instead of depending on 500ms mtimes.
test/codex-cli-sync.test.ts:611-615 and test/codex-cli-sync.test.ts:691-695 depend on the tmp filesystem preserving a 500ms mtime delta after utimes(). on coarser or rounded filesystems, that delta can collapse and make these windows-selection regressions flaky. please mock the stat/timestamp read path instead, like test/codex-cli-sync.test.ts:883-1065 already does, so the assertion stays deterministic.
As per coding guidelines, test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Also applies to: 691-695
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/codex-cli-sync.test.ts` around lines 611 - 615, The test relies on a
500ms mtime delta set by utimes/writeFile (using accountsPath and
targetStoragePath) which is flaky on some filesystems; instead mock the
timestamp/stat read path the same way as the existing block around lines
883-1065 does (stub fs.stat or the module/function that reads file mtimes in the
sync code) so the selection logic sees deterministic mtimes for sourceTime and
targetTime; update the assertions to use the mocked stat results and remove
dependence on actual utimes/writeFile timing.
| async function removeWithRetry(targetPath: string): Promise<void> { | ||
| for (let attempt = 0; attempt < 5; attempt += 1) { | ||
| try { | ||
| await fs.rm(targetPath, { recursive: true, force: true }); | ||
| return; | ||
| } catch (error) { | ||
| const code = (error as NodeJS.ErrnoException).code; | ||
| if ( | ||
| (code !== "EBUSY" && code !== "EPERM" && code !== "ENOTEMPTY") || | ||
| attempt === 4 | ||
| ) { | ||
| throw error; | ||
| } | ||
| await new Promise((resolve) => setTimeout(resolve, 25 * 2 ** attempt)); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
removeWithRetry duplicates test/helpers/remove-with-retry.js.
this file already imports removeWithRetry from ./helpers/remove-with-retry.js in other test files (see test/storage-recovery-paths.test.ts:6). consider importing the shared helper instead of duplicating the implementation.
♻️ proposed fix - use shared helper
+import { removeWithRetry } from "./helpers/remove-with-retry.js";
+
import { promises as fs } from "node:fs";
import { tmpdir } from "node:os";
import { join } from "node:path";
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
-
-async function removeWithRetry(targetPath: string): Promise<void> {
- for (let attempt = 0; attempt < 5; attempt += 1) {
- try {
- await fs.rm(targetPath, { recursive: true, force: true });
- return;
- } catch (error) {
- const code = (error as NodeJS.ErrnoException).code;
- if (
- (code !== "EBUSY" && code !== "EPERM" && code !== "ENOTEMPTY") ||
- attempt === 4
- ) {
- throw error;
- }
- await new Promise((resolve) => setTimeout(resolve, 25 * 2 ** attempt));
- }
- }
-}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/recovery.test.ts` around lines 49 - 65, The removeWithRetry function is
duplicated; remove the local async function removeWithRetry from this test and
instead import the shared helper (named removeWithRetry) from the existing
helpers module (./helpers/remove-with-retry.js) and update any local references
to use that imported symbol; ensure the import is added at the top of the file
and the duplicate function is deleted so the test uses the centralized
implementation.
| it.each(["EPERM", "EBUSY"] as const)( | ||
| "retries transient %s when clearing flagged storage", | ||
| async (code) => { | ||
| await saveFlaggedAccounts({ | ||
| version: 1, | ||
| accounts: [ | ||
| { | ||
| refreshToken: "stale-primary", | ||
| flaggedAt: 1, | ||
| addedAt: 1, | ||
| lastUsed: 1, | ||
| }, | ||
| ], | ||
| }); | ||
|
|
||
| await expect(clearFlaggedAccounts()).rejects.toThrow( | ||
| "EPERM primary delete", | ||
| ); | ||
| const flaggedPath = getFlaggedAccountsPath(); | ||
| const realUnlink = fs.unlink.bind(fs); | ||
| let failedOnce = false; | ||
| const unlinkSpy = vi | ||
| .spyOn(fs, "unlink") | ||
| .mockImplementation(async (targetPath) => { | ||
| if (targetPath === flaggedPath && !failedOnce) { | ||
| failedOnce = true; | ||
| const error = new Error("locked") as NodeJS.ErrnoException; | ||
| error.code = code; | ||
| throw error; | ||
| } | ||
| return realUnlink(targetPath); | ||
| }); | ||
|
|
||
| try { | ||
| await expect(clearFlaggedAccounts()).resolves.toBe(true); | ||
| expect(existsSync(flaggedPath)).toBe(false); | ||
| expect( | ||
| unlinkSpy.mock.calls.filter( | ||
| ([targetPath]) => targetPath === flaggedPath, | ||
| ), | ||
| ).toHaveLength(2); | ||
| } finally { | ||
| unlinkSpy.mockRestore(); | ||
| } | ||
| }, | ||
| ); |
There was a problem hiding this comment.
make the eperm cases platform-aware.
test/storage-flagged.test.ts:254-341 expects EPERM to retry exactly like EBUSY, but lib/storage.ts:2040-2048 only treats EPERM as retryable on win32. these cases will fail on linux/macOS and make the suite nondeterministic across runners. gate EPERM to win32 and keep a cross-platform retry case on EBUSY or EAGAIN.
As per coding guidelines, test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.
Also applies to: 298-341
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/storage-flagged.test.ts` around lines 254 - 296, The test currently
asserts that both "EPERM" and "EBUSY" retry, but EPERM is only retryable on
Windows in the implementation (clearFlaggedAccounts), making the test flaky on
non-win32 platforms; update the test in storage-flagged.test.ts to be
platform-aware by: keep a cross-platform retry test for "EBUSY" (or "EAGAIN")
only, and either skip or restrict the "EPERM" case to when process.platform ===
"win32" (or use os.platform()), or split into two it blocks where the EPERM test
is wrapped in a platform guard; reference the test block using the existing
it.each and the clearFlaggedAccounts/getFlaggedAccountsPath symbols so the
behavior matches lib/storage.ts.
Summary
Add manual rollback support for Codex CLI sync operations.
Current head
dev4f60292Latest follow-up on this head
Validation
npm test -- test/codex-cli-sync.test.tsnpm run lintnpm run typechecknpm run buildnpm testNotes
The remaining live review noise should be re-evaluated against
4f60292. The direct regression requested for a newer manual changed run without a checkpoint is now covered on this head.note: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
this PR adds full manual rollback support for codex CLI sync operations: before a manual apply, a named backup snapshot is captured and stored in the sync run record; a scan of the ndjson sync-history log can then locate that snapshot and restore it via
rollbackLatestCodexCliSync. alongside rollback, the PR ships a newsync-history.tsmodule (mutex-serialised ndjson log with backward-scanning tail reads), adestructive-actions.tsmodule (delete-account, delete-saved-accounts, reset-local-state with proper index rebasing), generation-guarded live-account-sync reloads, and wires all of this into the interactive login loop incodex-manager.ts.key observations:
inFlightCodexCliSyncRollbackPromiseserialisation guard has a concurrency window: two concurrent callers that bothawait getLatestCodexCliSyncRollbackPlan()before either sets the guard can both proceed and write the storage file twice — see inline comment onrollbackLatestCodexCliSyncloadRollbackSnapshothas no retry on transient windows filesystem errors (EBUSY/EPERM) unlike the save path — can surface spurious "unavailable" results on windows under filesystem contentionnormalizePathForComparisondeterminesisWindowsPathfrom the raw (pre-normalised) input; a posix path containing a literal\would be incorrectly lowercased, making comparison case-insensitivedeleteAccountAtIndexusesfailurePolicy:"error"for its pre-delete snapshot, so a windows snapshot failure silently blocks the delete;deleteSavedAccountsis more lenientrollbackLatestCodexCliSyncis missing; all other new logic has solid coverageConfidence Score: 3/5
Important Files Changed
Sequence Diagram
sequenceDiagram participant UI as codex-manager (UI) participant Sync as codex-cli/sync.ts participant History as sync-history.ts participant Storage as storage.ts participant Disk as Filesystem Note over UI,Disk: Manual Apply (trigger="manual") UI->>Sync: applyCodexCliSyncToStorage(current, {trigger:"manual"}) Sync->>Storage: captureRollbackSnapshot(current, targetPath) Storage->>Disk: snapshotAccountStorage → .bak file Disk-->>Storage: NamedBackupMetadata {name, path} Storage-->>Sync: CodexCliSyncRollbackSnapshot Sync->>Sync: createSyncRun({rollbackSnapshot, trigger:"manual"}) Sync-->>UI: {storage, changed:true, pendingRun} UI->>Storage: saveAccounts(storage) Storage->>Disk: write accounts.json UI->>Sync: commitPendingCodexCliSyncRun(pendingRun) Sync->>Sync: publishCodexCliSyncRun(run, revision) Sync--)History: appendSyncHistoryEntry (fire-and-forget) History->>Disk: append ndjson + write latest.json Note over UI,Disk: Manual Rollback UI->>Sync: getLatestCodexCliSyncRollbackPlan() Sync->>Sync: findLatestManualRollbackRun() — check in-memory, then readSyncHistory Sync->>History: readSyncHistory({kind:"codex-cli-sync", limit:200}) History->>Disk: readHistoryTail (backward chunk scan) Disk-->>History: entries History-->>Sync: CodexCliSyncRun[] Sync->>Disk: fs.readFile(snapshot.path) Disk-->>Sync: serialised AccountStorageV3 Sync-->>UI: CodexCliSyncRollbackPlan {status:"ready", storage} UI->>Sync: rollbackLatestCodexCliSync(plan) Sync->>Sync: check inFlightCodexCliSyncRollbackPromise Sync->>Storage: saveRollbackStorageWithRetry(rollbackStorage) Storage->>Disk: saveAccounts (with EBUSY/EPERM retry on win32) Sync->>Sync: publishCodexCliSyncRun(rollback run, newRevision) Sync--)History: appendSyncHistoryEntry(rollback entry) Sync-->>UI: {status:"restored", snapshot, accountCount}Comments Outside Diff (1)
lib/codex-cli/sync.ts, line 591-663 (link)rollback in-flight guard has a concurrency window
when no
planis passed, the function doesawait getLatestCodexCliSyncRollbackPlan()before checkinginFlightCodexCliSyncRollbackPromise. two concurrent callers can both enter theawait, both resume finding the guard stillnull, and both independently create rollback promises — writing the storage file twice and overwriting the firstinFlightCodexCliSyncRollbackPromiseassignment so only the second promise clears it.fix: resolve the plan synchronously when possible, or check
inFlightCodexCliSyncRollbackPromisebefore the async plan resolution and return early:there is still a narrow window between the two guards, but it eliminates the most common concurrent-caller path. full serialisation requires moving plan resolution inside the in-flight promise itself.
Prompt To Fix All With AI
Last reviewed commit: "test(sync): cover ne..."